I'm not familiar with hello retries, but do I understand correctly that this PR also enables support for external PSK with initial authentication (so one could use PSK instead of certs)?

@sshock That's correct.

IIRC, HelloRetryRequest is a used for renegotiating the ECDHE algorithm being used in case the client's initial offer did not match what the server can handle. But when PSK is used, it would be unlikely that the client and server to initially disagree on which ECDHE algorithm to be used considering that they've already agreed on the PSK to be used.

That's what the original comment says.

I'm not sure if we are going to merge this PR soon or ever, but I believe it works.

I'm not sure if we are going to merge this PR soon or ever, but I believe it works.

Ok. I may do some testing with it and let you know how it works for me.

I created a new PR with these changes rebased against the latest master and all conflicts resolved.

I tried to be careful, but there were a lot of conflicts, including 2 lines of code that I was unable to find a corresponding location for in the latest code, so I don't know if they are needed or not.

I am able to confirm that the new pre-shared-key test is passing, both with test-minicrypto.t and test-openssl.t. I haven't done any further testing yet.

I modified the cli tool to support PSK, but it's seg faulting on the server side after handshake while trying to send a NewSessionTicket.

It's calling send_session_ticket and trying to pass tls->key_share->id into encode_session_identifier, but tls->key_share is NULL:

    /* build the raw nsk */
    ret = encode_session_identifier(tls->ctx, &session_id, ticket_age_add, ptls_iovec_init(NULL, 0), tls->key_schedule,
                                    tls->server_name, tls->key_share->id, tls->cipher_suite->id, tls->negotiated_protocol);

I'm not a TLS expert (yet 😄), so I'm not even sure if it should be sending a new session ticket, but I believe it is doing that because can_send_session_ticket gets set to true in server_handle_hello on this line: tls->server.can_send_session_ticket = ch->psk.ke_modes != 0;.

And the reason tls->key_share is NULL is probably because server_handle_hello deliberately skips setting tls->key_share = key_share.algorithm; when the mode is HANDSHAKE_MODE_PSK.

Any advice on how this should be corrected?

Sounds like we have a bug in send_session_ticket function, though I think the server doesn't need to send a session ticket when the endpoints already have PSK shared.

By setting ptls_context_t::ticket_lifetime to zero, picotls will stop calling send_session_ticket (see https://github.com/h2o/picotls/blob/master/lib/picotls.c#L4761). Hopefully that would be the quick fix.

Sounds like we have a bug in send_session_ticket function, though I think the server doesn't need to send a session ticket when the endpoints already have PSK shared.

By setting ptls_context_t::ticket_lifetime to zero, picotls will stop calling send_session_ticket (see https://github.com/h2o/picotls/blob/master/lib/picotls.c#L4761). Hopefully that would be the quick fix.

Thanks @kazuho. This workaround worked so that I am no longer encountering a seg fault on the server side.

But now I am running into a PTLS_ALERT_ILLEGAL_PARAMETER on the client side, because the server is sending an early data extension (although the length is 0), but the client isn't expecting that:

        case PTLS_EXTENSION_TYPE_EARLY_DATA:
            if (!tls->client.using_early_data) {
                ret = PTLS_ALERT_ILLEGAL_PARAMETER;
                goto Exit;
            }
            skip_early_data = 0;
            break;

The server is sending a 0-length early data here in the /* send EncryptedExtensions */ block:

            if (tls->pending_handshake_secret != NULL)
                buffer_push_extension(sendbuf, PTLS_EXTENSION_TYPE_EARLY_DATA, {});

tls->pending_handshake_secret is set to "h" contains 0x30 bytes calculated from derive_secret(..., "c hs traffic") a few lines earlier in the /* create protection contexts for the handshake */, and the reason it is going into that section is because an earlier section allocs memory for tls->pending_handshake_secret, and the reason it does that is because an earlier section sets psk_index when it calls try_psk_handshake.

So one of these implications doesn't make sense (or the tls->client.using_early_data should have gotten set):

• We're doing external PSK
• -> so it goes into the /* try external psk handshake */ section
• -> so it calls try_psk_handshake, which sets psk_index to 0
• -> so it allocates memory for tls->pending_handshake_secret (and calls derive_exporter_secret and setup_traffic_protection)
• -> so it initializes tls->pending_handshake_secret with derive_secret
• -> so the server sends an empty early data in the EncryptedExtensions

I'll have to go study the RFC, because I don't understand how handshake secrets and traffic protection relate to early data.

Update: I understand now from RFC sections 4.2.10 and 4.2.11 that the early_data extension is just an indicator and is supposed to be empty (the only exception is in a NewSessionTicket where it only contains a uint32 max_early_data_size). The early_data extension from the server indicates that it accepts the early data from the client, so it is indeed incorrect for the server to send that when the client has not.

Update: I found the bug. The new code in try_psk_handshake for external psk is setting *accept_early_data = 1 unconditionally; it needs to only do that if the client sent early data (if ch->psk.early_data_indication). I'll be adding this one-line fix to my PR sometime today.

Here's what I noticed when I tried to connect between sample client and server with picotls, wolfssl, and openssl:

One key difference I noticed when viewing everything in wireshark was that wolfssl and openssl servers respond with a key_share whereas picotls doesn't. But I think that may be because picotls is choosing to do PSK-only mode, whereas the others are doing PSK with (EC)DHE, so maybe it's fine.

This is the block of code on the openssl server that is failing (with picotls client) is here in tls_parse_ctos_psk():

    if (PACKET_remaining(&binder) != hashsize) {
        SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_CTOS_PSK,
                 SSL_R_BAD_EXTENSION);
        goto err;
    }

I think it's because the PSK Binder hmac is 48 bytes (whereas with wolfssl and openssl it is 32 bytes).

It appears the picotls client is using SHA-384 here, but I think it needs to default to SHA-256. From RFC 8446 section 4.2.11:

Each PSK is associated with a single Hash algorithm.  For PSKs
   established via the ticket mechanism (Section 4.6.1), this is the KDF
   Hash algorithm on the connection where the ticket was established.
   For externally established PSKs, the Hash algorithm MUST be set when
   the PSK is established or default to SHA-256 if no such algorithm is
   defined.  The server MUST ensure that it selects a compatible PSK
   (if any) and cipher suite.

Update: What I've learned about PSK and the cipher suite:

From section 4.2.11 mentioned above, I've learned that the hash algorithm MUST be established beforehand or default to SHA-256.

From section 4.2.10, I've learned that in order to accept early data, the server must choose the cipher suite associated with the selected PSK, which for an external PSK was "provisioned along with the key". But the RFC doesn't indicate exactly how or where clients and servers should store this associated cipher suite.

Which means, unless the client and server have predetermined beforehand which cipher suite to use with a specific external PSK, early data can't work. However, external PSK can still work fine (without early data), but the client must default to SHA-256 for the hash algorithm, and the server must select a compatible cipher suite (e.g., TLS_AES_128_GCM_SHA256).

I think I found another issue: picotls needs to use "ext binder" not "res binder" for the binder key label in this case where we are doing external PSK not resumption.

Update: This is confirmed in Section 7.1 of the RFC:

For the computation of the binder_key, the label is "ext binder" for external PSKs (those provisioned outside of TLS) and "res binder" for resumption PSKs

I'll work on fixing this and the hash algorithm in my PR. It should be pretty easy.

I made two fixes to the client side, and now the picotls cli client is able to connect and communicate with both wolfssl and openssl servers:

• Use correct binder key label
• Use SHA256 with PSK make it configurable (but default to SHA256)

I made the following fixes to the server side today:

• Use correct binder key label on server
  
• Fix bug that was causing server to send early data indicator
• Select PSK compatible cipher suite on server (I would love feedback on how to do this in a cleaner way)
• Only accept early data if using the first PSK (may not have needed this; see commit comment)

As a result, things have improved in my interop testing:

Good news. I was able to do more testing with wolfssl and openssl clients and get them working against picotls server with only one real failure remaining in one scenario (when wolfssl presents both ke modes). But at this point I'm ready to call it good, and that could even be a bug in wolfssl.

@kazuho could we consider merging my PR? It would be great if I didn't have to maintain it in a fork.

@sshock Thank you for all the work and coming up with a fix. I'll take a look at your code next work.

should we close this out since we have #481 ?